Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for GitHub-hosted AI models across various components and standardizes the naming of embedding dimension environment variables.
- Introduces branches for GitHub Models in clients, routes, dependencies, and evaluation modules.
- Refactors environment variable names and dynamic attribute access to support multiple model hosts within the application.
Reviewed Changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/conftest.py | Renamed an embedding dimension env var to standardize naming. |
| src/backend/fastapi_app/update_embeddings.py | Added a new branch for GitHub to select the appropriate column. |
| src/backend/fastapi_app/routes/api_routes.py | Changed to dynamic attribute retrieval for embedding column. |
| src/backend/fastapi_app/openai_clients.py | Added support for GitHub Models for both chat and embeddings. |
| src/backend/fastapi_app/dependencies.py | Extended GitHub support for embedding and chat configuration. |
| evals/generate_ground_truth.py | Updated error messages to reflect GitHub Models are not supported. |
| evals/evaluate.py | Included GitHub branch for evaluation to prevent unsupported usage. |
Files not reviewed (2)
- .env.sample: Language not supported
- infra/main.bicep: Language not supported
Comments suppressed due to low confidence (3)
tests/conftest.py:98
- Ensure that the updated environment variable naming in tests ('OPENAICOM_EMBED_DIMENSIONS') matches the standardized naming in application code for consistency.
monkeypatch_session.setenv("OPENAICOM_EMBED_DIMENSIONS", "1024")
evals/generate_ground_truth.py:106
- [nitpick] The error message for GitHub Models now mirrors other providers; consider including additional context or documentation to help users understand current support limitations.
raise NotImplementedError("GitHub Models is not supported. Switch to Azure or OpenAI.com")
src/backend/fastapi_app/routes/api_routes.py:71
- Dynamic attribute retrieval is a good refactor; however, verify that all Item instances are guaranteed to have the attribute specified by context.embedding_column to avoid potential AttributeError at runtime.
getattr(item, context.embedding_column)
| base_url=os.getenv("OLLAMA_ENDPOINT"), | ||
| api_key="nokeyneeded", | ||
| ) | ||
| elif OPENAI_CHAT_HOST == "github": |
There was a problem hiding this comment.
[nitpick] Consider documenting the GitHub Models branch in the client setup to clarify why no explicit model parameter is passed during initialization, ensuring future maintainability.
Suggested change
| elif OPENAI_CHAT_HOST == "github": | |
| elif OPENAI_CHAT_HOST == "github": | |
| # The GitHub Models branch does not require an explicit model parameter during initialization. | |
| # Instead, it relies on the GITHUB_BASE_URL and GITHUB_MODEL environment variables to configure | |
| # the base URL and model name, respectively. This design choice ensures flexibility and avoids | |
| # hardcoding model details in the code. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This pull request introduces support for GitHub-hosted AI models in various parts of the codebase and includes a minor refactor to standardize the naming of embedding dimension environment variables.
Does this introduce a breaking change?
When developers merge from main and run the server, azd up, or azd deploy, will this produce an error?
If you're not sure, try it out on an old environment.
Type of change
Code quality checklist
See CONTRIBUTING.md for more details.
python -m pytest).python -m pytest --covto verify 100% coverage of added linespython -m mypyto check for type errorsruffmanually on my code.